New config service#55
Conversation
|
|
||
| private loadConfiguration(): IAppConfiguration { | ||
| const config = yaml.load( | ||
| readFileSync(join(__dirname, './config.yaml'), 'utf8') |
|
|
||
| const keycloakConfig: IKeycloakConfiguration = configValue; | ||
|
|
||
| const local = { |
There was a problem hiding this comment.
what does local mean in this context?
There was a problem hiding this comment.
it means from local enviroment
| private loadConfiguration(): IAppConfiguration { | ||
| const config = yaml.load( | ||
| readFileSync(join(__dirname, './config.yaml'), 'utf8') | ||
| ) as Record<string, unknown>; |
There was a problem hiding this comment.
what's the purpose of as Record<string, unknown> here?
|
|
||
| const localConfig: ILocalConfiguration = localValue; | ||
|
|
||
| return { |
There was a problem hiding this comment.
So it seems that you only load keycloak config from file and the rest from ENV. Why is that?
There was a problem hiding this comment.
Does the keycloak config contain sensitive data? I thought they were not sensitive.
| }; | ||
| } | ||
|
|
||
| getKcConfig(): IKeycloakConfiguration { |
There was a problem hiding this comment.
There's really no point in sacrificing readability for the number of characters. getKeycloakConfig is better in this case
| @@ -0,0 +1,47 @@ | |||
| import * as Joi from 'joi'; | |||
|
|
|||
| export interface IKeycloakConfiguration { | |||
There was a problem hiding this comment.
Do not prefix interfaces with I. Homework: do some research online for reasons why. Let's discuss more after you get some insights
There was a problem hiding this comment.
from all i found this 2 make sense
1, Readability and Expressiveness: Some argue that a well-named interface should be clear and expressive on its own without needing a prefix. If the interface name is appropriately descriptive, the 'I' might be redundant.
2, Unnecessarily Revealing Implementation Details: The 'I' prefix might expose the underlying implementation details, which could be considered a violation of the abstraction principle. Interfaces should define a contract without revealing how it will be implemented.
peterrogov
left a comment
peterrogov
left a comment
There was a problem hiding this comment.
-
Naming for config sections. Needs revisiting.
-
Logic of merging the config file data with ENV is still a little flawed. See my comments before for this service.
Config YML
keycloak:
realm: 'humanitech'
adminPassword: '123456'
database:
host: '127.0.0.1'ENV file
KC_ADMIN_PASSWORD=123789Sketch code to merge and validate
const configYmlToEnvMap = {
"keycloak.adminPassword": "KC_ADMIN_PASSWORD"
// ... and more as required ...
}
const config = loadYmlFileAndValidate(); // load and validate the config from file
// Override from ENV
Object.entries(TaskProcessor).forEach(([ymlKey, envName]) => {
if(process.env[envName]) {
// think of a way how to set a property deep in the tree using its path
// for example, lodash.set does this job pretty well
setConfig(config, ymlKey, process.env[envName]);
}
});
// after overriding from ENV the config could receive invalid values
// one more round of schema based validation is required
const finalConfig = validate(config);
// Now you have a complete, merged, validated config. | throw new Error(`Config validation error: ${localError.message}`); | ||
| for (const key in yamlConfig.local) { | ||
| if (process.env[key]) { | ||
| yamlConfig.local[key] = process.env[key]; |
There was a problem hiding this comment.
what about validation? what if there's garbage in ENV?
There was a problem hiding this comment.
The type of variables that come from the environment (process.env) is always a string, but I used dto, that will handle the validation for the whole object. so if I didn't get you, you can correct me
| grantType: 'password' | ||
| clientId: 'admin-cli' | ||
|
|
||
| local: |
There was a problem hiding this comment.
why local? what does it mean? if there's a need for sections, why not to make them reasonable, like keycloak, database, etc?
| DB_DATABASE: 'mydb' | ||
| POSTGRES_DATA: '/path/to/postgres/data' | ||
| KEYCLOAK_ADMIN: 'admin-user' | ||
| KEYCLOAK_ADMIN_PASSWORD: 'admin-password' |
There was a problem hiding this comment.
why is there a section for keycloak config and yet this parameter is stored here?
|
Kudos, SonarCloud Quality Gate passed! |
3 participants
No description provided.